@codex review

Code review

Verified: built ModularPipelines.sln (core) and ran ModuleResultContractTests on the PR branch — 0 errors/warnings, 8/8 tests pass, matching the PR description.

No blocking bugs found. ModuleResult<T>.Value correctly resolves to the pattern-matched Success type's hiding property inside the switch (no accidental recursion), and the T?→T narrowing compiles cleanly for both reference and value T (confirmed by the successful build, e.g. Module<int> in the test suite).

Two non-blocking suggestions:

1. Member hiding on Success.Value is a readability trap. Success declares public new T? Value { get; init; }, hiding the base ModuleResult<T>.Value (the new throwing accessor). That means .Value behaves differently depending only on the static type of the reference — non-throwing/nullable when the variable is typed as Success, throwing when typed as ModuleResult<T>, even for the exact same object. This is a classic C# footgun (new hiding + static dispatch), and it's easy for a future contributor to hit a silent behavior change just by narrowing/widening a variable's declared type. Since the nullable-accessor is really only needed for the Success { Value: var x } pattern-matching idiom already used in the docs, consider renaming it (e.g. RawValue) to avoid the same identifier meaning two different things, rather than relying on hiding + new.

2. Follow-up opportunity, not required here: the new Value accessor is exactly the safe replacement for the .ValueOrDefault! null-forgiving pattern used throughout src/ModularPipelines.Build/Modules/* and src/ModularPipelines.Examples/Modules/** (e.g. FindProjectDependenciesModule.cs, PackProjectsModule.cs, the Examples/Modules/Approvals|Launch|Propulsion|Systems modules). Those call sites are exactly the "required dependency" case this PR is designed for, and migrating them would let the codebase drop the null-forgiveness operator in favor of a real, module-aware exception message. Worth a follow-up PR rather than blocking this one, since it's unrelated to the current diff.

Nice addition overall — the Value/ValueOrDefault split is clean, the exception messages are genuinely useful for debugging, and the docs/README updates match the new API well.

Addressed the P2 record-formatting finding in 9fd80c5192. Validation: focused ToString regressions 4/4, full ModuleResultContractTests 12/12, scoped formatting/analyzers clean, core Release build clean (228 existing warnings). @codex review

Addressed named-argument source compatibility in c5668608fe after rebasing onto current main. The focused regression test passes, scoped formatting/analyzers pass, and the core Release build passes with 0 errors (228 existing warnings).

@codex review

Restored the positional record's named Deconstruct(Value: ...) source API with regression coverage and a documented SA1313 compatibility suppression. @codex review

Code review

Verified on latest commit 4265d6a ("fix(results): preserve deconstruct API"): rebuilt ModularPipelines.sln (core, 0 errors/0 warnings) and ran ModuleResultContractTests — 13/13 pass, including the new Success_Value_ReturnsValue test that exercises success.Deconstruct(Value: out var deconstructedValue).

This round's change

This commit restores the manual Deconstruct(out T? Value) on ModuleResult<T>.Success (src/ModularPipelines/Models/ModuleResult.cs:365-371) that was implicitly generated back when Success was a positional record. I confirmed no other call site in the repo relied on deconstructing Success (grep for Deconstruct/is ... Success( turned up nothing outside this file and the new test), so this is a clean, low-risk restoration of source compatibility rather than a behavior change — good fix.

Status of all prior review rounds

Every previously-raised issue across this PR's iterations is now resolved and independently re-verified:

• Record-formatting-throws-during-ToString() (P2) — fixed via the PrintMembers override, still load-bearing, still passing its 4 regression tests.
• Named-argument source compatibility (new Success(Value: 42)) — fixed, still passing.
• Positional Deconstruct source compatibility — fixed in this commit, now covered by a regression assertion.

Still-open, non-blocking design note (carried over, not new)

Success still hides the base's throwing Value with public new T? Value { get; init; }. As raised in earlier rounds, this means .Value throws-on-null when statically typed as ModuleResult<T> but doesn't when statically typed as Success — for the same object. This is a deliberate, working trade-off to keep the Success { Value: var x } pattern-matching idiom, and not a blocker, but worth a RawValue-style rename or at least a doc-comment callout before this shape sees wider adoption (e.g. the suggested ValueOrDefault! → .Value migration in src/ModularPipelines.Build/Examples).

No new bugs found. This is a solid, well-tested addition — nice work tracking down each source-compatibility edge case across the review rounds.

Synced the required-value example into README_Template.md in 0d652e19d7, so GenerateReadMeModule preserves the checked README change. Exact snippets compared equal; no stale buildResult.ValueOrDefault! example remains.

@codex review

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 0d652e19d7

Code review

Verified on latest commit 0d652e19d7 ("docs(readme): sync required value example"): rebuilt ModularPipelines.sln (core, 0 errors/0 warnings) and ran ModuleResultContractTests — 13/13 pass. src/ModularPipelines/Models/ModuleResult.cs is unchanged since the previously-verified 4265d6a, so this round is docs-only.

This round's change

This commit syncs the required-value example from README.md into README_Template.md (buildResult.ValueOrDefault!.OutputPath → buildResult.Value.OutputPath). I checked GenerateReadMeModule (src/ModularPipelines.Build/Modules/GenerateReadMeModule.cs:26-56): it reads README_Template.md, substitutes only the %%% AVAILABLE MODULES PLACEHOLDER %%% token, and diffs the result against the checked-in README.md, failing the build step if they don't match. Since the example text isn't part of the substituted placeholder, the two files had to be updated in lockstep — this fix is correct and closes the drift that would otherwise have broken that check.

Status of all prior review rounds

Every previously-raised issue across this PR's iterations remains resolved:

• Record-formatting-throws-during-ToString() — fixed via the PrintMembers override, still passing its regression tests.
• Named-argument source compatibility (new Success(Value: 42)) — fixed, still passing.
• Positional Deconstruct source compatibility — fixed, still covered by a regression assertion.

Still-open, non-blocking design note (carried over, not new)

Success still hides the base's throwing Value with public new T? Value { get; init; } (ModuleResult.cs:362). As raised in earlier rounds: .Value throws-on-null when statically typed as ModuleResult<T> but doesn't when statically typed as Success, for the same object — a static-dispatch footgun that's easy to trip over when narrowing/widening a variable's declared type during a future refactor. This is a deliberate trade-off to keep the Success { Value: var x } pattern-matching idiom working, and I'm not blocking on it again, but it's the one item from this review series that hasn't been addressed — worth a RawValue-style rename or a doc-comment callout before this API sees the suggested ValueOrDefault! → .Value migration across src/ModularPipelines.Build/Examples.

No new bugs found. This is a well-tested, source-compatible addition — nice work tracking down every edge case across the review rounds.

Rebased onto current main; new head 5eef295306. Whole-PR patch-id remained exact: 476c14c23013fe15585957e42559e2577d378b3c.

Validation after rebase:

• ModuleResultContractTests: 13/13 passed
• ModularPipelines.sln Release build: 0 errors (227 pre-existing warnings)
• docs production build: passed
• README and README_Template.md required-value snippets: exact match
• stale buildResult.ValueOrDefault!: absent
• git diff --check: passed

@codex review

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 5eef295306

Rebased onto current main after #3564 merged. Patch equivalence preserved (476c14c23013fe15585957e42559e2577d378b3c). New head: 9e24fdd8e4.

@codex review

Code review

Verified on latest head 9e24fdd8e4: this commit is a rebase-only update onto current main (after #3564 merged). Confirmed independently via git patch-id --stable on main...pr-head — the resulting patch-id (476c14c23013fe1...) is byte-identical to the previously-reviewed 5eef2953 commit. No source lines changed.

Since the diff content is unchanged, the prior review round stands in full:

• Record ToString()/logging throwing via the Value accessor — fixed via ModuleResult<T>.PrintMembers.
• Named-argument source compatibility (new Success(Value: 42)) — fixed.
• Positional Deconstruct source compatibility — fixed, regression-tested.
• README/README_Template.md required-value example drift — fixed, both files match.

Still-open, non-blocking design note (carried over, not new): ModuleResult<T>.Success still hides the base's throwing Value with public new T? Value { get; init; }. .Value throws-on-null when statically typed as ModuleResult<T> but not when statically typed as Success, for the same object — a static-dispatch footgun if a future refactor narrows/widens a variable's declared type. This remains a deliberate trade-off to keep the case Success { Value: var x } pattern-matching idiom working, and isn't a blocker — but a RawValue-style rename (or at least an XML-doc callout on Success.Value) would remove the ambiguity before the suggested ValueOrDefault! → .Value migration lands elsewhere in the codebase (src/ModularPipelines.Build/Examples), since call sites there will otherwise silently get different throw behavior depending on which static type they hold the result as.

No new bugs found. Nothing to re-verify beyond confirming the rebase is patch-equivalent — approving from a correctness standpoint.

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: a07f310a00

Code review

Verified on latest head a07f310a00 (merge of main into the branch): rebuilt ModularPipelines.sln (core, 0 errors/0 warnings) and ran ModuleResultContractTests — 13/13 pass.

This round's change

The only substantive change since the last review round is a 5-line XML <remarks> doc comment on Success.Value (

Status of all prior review rounds

Every issue raised across this PR's review history remains resolved:

• Record ToString()/logging throwing via the Value accessor — fixed via the PrintMembers override.
• Named-argument source compatibility (new Success(Value: 42)) — fixed.
• Positional Deconstruct source compatibility — fixed, regression-tested.
• README/README_Template.md required-value example drift — fixed, both files match.
• Success.Value static-dispatch footgun — now documented (this round), a reasonable resolution given the constraint of preserving the existing pattern-matching idiom.

No new bugs found. Nothing outstanding from this review series — the design is sound, well-tested, and now well-documented.

Code review

Verified on latest head f4fe14c57d (merge of main into the branch): this commit only pulls in unrelated upstream changes (#3569, #3577, #3578). git diff a07f310a00..f4fe14c57d -- src/ModularPipelines/Models/ModuleResult.cs is empty — no lines authored by this PR have changed since the last review round.

Every issue raised across this PR's review history remains resolved, and nothing new was introduced by the merge:

• Record ToString()/logging throwing via the Value accessor — fixed via the PrintMembers override.
• Named-argument source compatibility (new Success(Value: 42)) — fixed.
• Positional Deconstruct source compatibility — fixed, regression-tested.
• README/README_Template.md required-value example drift — fixed, both files match.
• Success.Value static-dispatch footgun — documented via XML <remarks>.

No new bugs found, no new design concerns. This PR is in good shape from a correctness and architecture standpoint.

Code review

Verified on latest head 1c989bb9 (merge into current main, wrapping PR head 56581be7): rebuilt ModularPipelines.sln (core) — 0 errors, 0 warnings. Ran ModuleResultContractTests — 19/19 pass. dotnet format --verify-no-changes --severity info is clean on the two changed source files.

This round's change: a real merge, not a no-op

Unlike the last several rounds, this head is not patch-equivalent to what was previously reviewed. 56581be7 is a merge from main that had genuine conflicts in ModuleResult.cs and ModuleResultContractTests.cs, because main had independently evolved ModuleResult<T>'s Failure/Skipped handling away from this PR's FailureWrapper/SkippedWrapper composition-over-a-private-field design, toward main's own new sealed record Failure(Exception Exception) : ModuleResult<T> / new sealed record Skipped(...) member-hiding design (hiding the base ModuleResult.Failure/Skipped the same way Success.Value already hides ModuleResult<T>.Value). This is the diff that actually needed scrutiny this round, since it's the first time these two designs were combined.

I diffed ModuleResult.cs and the test file against the last-reviewed commit (f4fe14c57d) and checked the resolution line by line:

• ModuleResult<T>.Value, Match, Switch all pattern-match on unqualified Failure/Skipped, which now correctly bind to the nested, hidden ModuleResult<T>.Failure/.Skipped types (not the base ModuleResult.Failure/.Skipped) — verified this resolves the same way the old FailureWrapper/SkippedWrapper cases did, and the new required-Value throw messages ("{ModuleName} failed: ...", "{ModuleName} was skipped: ...") still work against the new type shapes.
• Implicit conversions (ModuleResult.Failure → ModuleResult<T>.Failure, same for Skipped) now do a field-by-field copy into the new nested-type instance rather than composing over the original — checked all 7 metadata fields (ModuleName, ModuleTypeName, ModuleDuration, ModuleStart, ModuleEnd, ModuleStatus, ModuleType) are carried across in both operators; nothing dropped.
• JSON: the new [JsonConverter(typeof(ModuleResultJsonConverterFactory))] attribute was correctly added directly to the nested Failure/Skipped/Success records (needed now that they're independently-identified sealed types rather than wrapper instances reusing the base's attribute), and a new ModuleResultVariantJsonConverter<T, TVariant> adapts the canonical ModuleResultJsonConverter<T> to each concrete nested type. The new tests (Concrete_Generic_Failure_Serializes_Through_Json, Concrete_Generic_Skipped_Serializes_Through_Json, Generic_Failure_RoundTrips_Through_Json, Generic_Skipped_RoundTrips_Through_Json) exercise exactly this path and pass.
• Docs correctly updated for the semantic consequence of this design: docs/fundamentals.md and docs/how-to/sharing-data.md changed their pattern-match samples from ModuleResult.Skipped/ModuleResult.Failure to ModuleResult<T>.Skipped/ModuleResult<T>.Failure. This matters because with the new hiding design, a ModuleResult<T> instance's runtime type is never actually ModuleResult.Failure/Skipped (those are unrelated sibling types, same as it was under the old wrapper design) — so case ModuleResult.Failure: against a ModuleResult<T> value would silently never match. Good catch updating this at the same time as the type resolution, since a stale doc sample here would have taught readers a pattern that quietly does nothing.

No new bugs found in the conflict resolution. The merge is a faithful combination of both branches' designs, is fully covered by new regression tests for the paths that changed shape (nested-type JSON serialization, nested-type pattern matching), and everything from prior rounds remains resolved:

• Record ToString()/logging throwing via the Value accessor — still fixed via PrintMembers.
• Named-argument / positional Deconstruct source compatibility on Success — unaffected by this merge, still intact.
• README/README_Template.md required-value example — unaffected, still in sync.

Still-open, non-blocking design note (carried over, not new)

Success.Value still intentionally hides the base's throwing ModuleResult<T>.Value via new, documented via XML <remarks>. Worth noting this merge extends the same hiding pattern to Failure/Skipped, so the codebase is now consistently using "nested type hides base type" as its idiom across all three variants rather than mixing wrapper-composition with hiding — a reasonable consistency win, though it slightly widens the surface area of the original footgun (four hidden identifiers now instead of one: Success.Value, ModuleResult<T>.Failure, ModuleResult<T>.Skipped, and their now-doc-updated pattern-matching implications). Not a blocker — the docs and tests already account for it — but if this shape sees further reuse elsewhere in the codebase, a short glossary note in fundamentals.md about "always match on ModuleResult<T>.X, never the non-generic ModuleResult.X, when you have a typed result" would preempt the exact mistake the doc fix in this commit just corrected.

Nice work getting a non-trivial conflicting merge landed cleanly with test coverage for the new shape.

Code review

Verified on latest head 66ce290b19609ec9fb9620b5ef0f8d054b58114d: confirmed via gh api .../compare/56581be7...66ce290b that the only changes since the last-reviewed round are unrelated upstream files pulled in by merging main (auto-generated Docker/Minikube/Pulumi CLI-options enums, generate-cli-options.yml, and the options-generator's own test/scraper files) — none of this PR's six files (ModuleResult.cs, ModuleResultContractTests.cs, README.md, README_Template.md, docs/docs/fundamentals.md, docs/docs/how-to/sharing-data.md) changed.

Rebuilt ModularPipelines.sln (core) from this head — 0 errors, 0 warnings — and ran the full ModuleResultContractTests suite via dotnet run --framework net10.0 -- --treenode-filter '/*/*/ModuleResultContractTests/*': 19/19 pass.

Status of all prior review rounds

Every issue raised across this PR's long review history remains resolved, and nothing new was introduced by this merge:

• Record ToString()/logging throwing via the Value accessor — fixed via the PrintMembers override.
• Named-argument source compatibility (new Success(Value: 42)) — fixed.
• Positional Deconstruct source compatibility on Success — fixed, regression-tested.
• README/README_Template.md required-value example drift — fixed, both files match.
• Success.Value hiding the base's throwing ModuleResult<T>.Value via new — documented via XML <remarks>; the same nested-type-hides-base-type idiom is now used consistently for Failure/Skipped too, following main's independent evolution of those variants. Still a design footgun worth a RawValue-style rename if this shape gets reused elsewhere, but not a blocker — it's explicitly documented and covered by tests.

No new bugs found. Architecture and design remain sound: the required-Value accessor with outcome-specific exception messages is a clean, well-tested replacement for the ValueOrDefault! null-forgiving pattern, and the docs/README are kept in sync with the API. This PR is in good shape and ready from a correctness standpoint.

Code review

Verified on latest head 9f631e2d7c: diffed the PR's six files (ModuleResult.cs, ModuleResultContractTests.cs, README.md, README_Template.md, docs/docs/fundamentals.md, docs/docs/how-to/sharing-data.md) against the last-reviewed head (66ce290b) and against current main. The only differences since the last review round are:

• Merge noise from main (context.Tools canonical API rename, PR Make context.Tools the canonical tool API #3574) that nets to zero against main — confirmed the PR's own diff vs main for README.md/README_Template.md/fundamentals.md is byte-identical to what was reviewed last round.
• A pure reordering of two test methods in ModuleResultContractTests.cs (Success_Value_ReturnsValue and Success_Constructor_PreservesValueNamedArgument swapped position) — no behavioral change.

src/ModularPipelines/Models/ModuleResult.cs is byte-identical to the last-reviewed version.

Rebuilt ModularPipelines.sln (core) — 0 errors, 0 warnings — and ran ModuleResultContractTests — 19/19 pass.

Status of all prior review rounds

Every issue raised across this PR's review history remains resolved, and nothing new was introduced:

• Record ToString()/logging throwing via the Value accessor — fixed via the PrintMembers override.
• Named-argument source compatibility (new Success(Value: 42)) — fixed.
• Positional Deconstruct source compatibility on Success — fixed, regression-tested.
• README/README_Template.md required-value example drift — fixed, both files match.
• Success.Value hiding the base's throwing ModuleResult<T>.Value via new — documented via XML <remarks>; still a design footgun worth a RawValue-style rename if this hiding idiom gets reused elsewhere, but not a blocker — it's explicitly documented and covered by tests.

No new bugs found, no new design concerns. This PR remains in good shape from a correctness and architecture standpoint.

@codex review

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 6578187726

Rebased onto current main; ModuleResult contract tests pass (19/19). @codex review

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 365ef1d24f

Code review

Verified on latest head 365ef1d24f ("fix(results): match generic result variants"): rebuilt ModularPipelines.sln (core) — 0 errors, 0 warnings. Ran ModuleResultContractTests (--treenode-filter '/*/*/ModuleResultContractTests/*') — 19/19 pass. dotnet format --verify-no-changes --severity info is clean on both changed source files.

This round's change

This commit is a small, necessary correctness fix on top of the earlier main-merge round: the ModuleResult<T>.Value switch expression (src/ModularPipelines/Models/ModuleResult.cs:229-239) still referenced FailureWrapper/SkippedWrapper, types that no longer exist after that merge replaced the wrapper-composition design with nested Failure/Skipped records that hide the base non-generic ones via new. Without this fix the switch's Failure/Skipped arms wouldn't have matched at all (falling through to the generic "has an unknown result type" branch, or simply failing to compile — either way, broken). Renaming the pattern types to Failure/Skipped correctly re-aligns the switch with the current type shape, and it's exercised by the existing Failure_Value_ThrowsWithModuleContext / Skipped_Value_ThrowsWithModuleContext tests, both of which pass.

Status of all prior review rounds

Every issue raised across this PR's long review history remains resolved:

• Record ToString()/logging throwing via the Value accessor — fixed via the PrintMembers override.
• Named-argument source compatibility (new Success(Value: 42)) — fixed.
• Positional Deconstruct source compatibility on Success — fixed, regression-tested.
• README/README_Template.md required-value example drift — fixed, both files match.

Still-open, non-blocking design note (carried over, not new): Success hides the base's throwing ModuleResult<T>.Value with public new T? Value { get; init; }. .Value throws-on-null when a reference is statically typed as ModuleResult<T> but returns null silently when statically typed as Success, for the same object. This is a deliberate trade-off to preserve the Success { Value: var x } pattern-matching idiom and the source-compatible constructor/deconstruct shape, and it's now explicitly documented via an XML <remarks> on Success.Value. Still worth a RawValue-style rename if this hiding idiom is reused elsewhere (it's already been extended to the generic Failure/Skipped variants), but not a blocker for this PR.

No new bugs found. This is a well-tested, correctly-fixed follow-up to the merge round — good catch tracking down the stale type references.